cmd/webhooks/webhooks_test.go (1)

27-83: Consider using subtests and asserting request payloads / response output.

This monolithic test verifies that each operation doesn't error, but it doesn't assert:

1. That the correct HTTP request body is sent (e.g., the create payload includes "name":"hook2").
2. That the printed output matches the server's response data.
3. Individual failures — if runList fails, the remaining operations are never exercised.

Using t.Run(...) subtests and inspecting r.Body in handlers (or capturing stdout) would strengthen coverage and make failures easier to diagnose.

cmd/experiments/experiments_extra_test.go (4)

486-511: Inconsistent stdout capture approach.

Lines 496–503 manually set up an os.Pipe to capture stdout, but testutil.CaptureStdout is used elsewhere in this same file (e.g., Lines 315, 347, 374, 411). Use the shared helper here too for consistency and to avoid the risk of leaking the replaced os.Stdout if the test panics before the restore.

♻️ Suggested fix

 	cmd := newGenerateTemplateCmd()
 	cmd.Flags().Set("name", "My Name")
 	cmd.Flags().Set("type", "test")
 
-	var stdout bytes.Buffer
-	orig := os.Stdout
-	r, w, _ := os.Pipe()
-	os.Stdout = w
-	require.NoError(t, runGenerateTemplate(cmd, nil))
-	_ = w.Close()
-	_, _ = stdout.ReadFrom(r)
-	_ = r.Close()
-	os.Stdout = orig
-	assert.Contains(t, stdout.String(), "My Name")
+	output := testutil.CaptureStdout(t, func() {
+		require.NoError(t, runGenerateTemplate(cmd, nil))
+	})
+	assert.Contains(t, output, "My Name")

---

698-707: Setting both terse and full simultaneously is under-asserted.

terse and full are mutually exclusive flags according to the PR description. The test sets both to true and only checks that no error is returned. Consider asserting the actual output or behaviour to verify which flag takes precedence (or that an error is raised for conflicting flags).

---

744-749: TestSearchHelpers only tests stdlib string operations on a struct literal.

This test constructs an api.Experiment and then calls strings.Contains/strings.ToLower on its fields — it does not invoke any helper function from the project. If there is a search-matching helper, test that; otherwise this can be removed alongside the similar assertions at Lines 588–592.

---

204-215: Clarify the intent of viper.Reset() after config setup.

viper.Reset() on Line 214 clears all in-memory Viper state immediately after SetupConfig writes it. This works only because the commands presumably re-read from the config file on disk. A brief comment would prevent future maintainers from "fixing" this by removing the reset.

cmd/goals/goals.go (2)

204-224: Nice ordering — confirmation before client initialisation avoids unnecessary auth on cancel.

Minor nit: fmt.Println("Cancelled") on line 208 bypasses the output layer. If the CLI ever needs to support --quiet suppression or structured output for cancellation messages, this would need updating. Not urgent — just something to keep in mind.

---

52-52: Consider using cmd.Context() instead of context.Background() throughout.

All five command handlers use context.Background(). Cobra's cmd.Context() respects signal-based cancellation (e.g., Ctrl+C), which is more idiomatic for CLI tools and would allow long-running API calls to be interrupted gracefully. This applies to lines 52, 96, 135, 181, and 217.

Example diff (apply similarly to the other four call sites)

-	goals, err := client.ListGoals(context.Background(), opts)
+	goals, err := client.ListGoals(cmd.Context(), opts)

cmd/experiments/experiments.go (1)

884-892: Consider warning before overwriting an existing file.

os.WriteFile on line 886 silently overwrites any existing file at outputPath. A quick os.Stat check and prompting (or requiring --force) would be consistent with the confirmation pattern already used in runDelete.

cmd/experiments/notes.go (4)

23-33: Inconsistent argument validation across sibling subcommands.

update and delete-all use cobra.ExactArgs(1), but list rolls its own check. Using Cobra's built-in validator keeps behaviour (error format, automatic usage hint) consistent and removes boilerplate.

Suggested fix

 	listCmd := &cobra.Command{
 		Use:     "list <experiment-id>",
 		Short:   "List notes for an experiment",
 		Long:    "Display all notes associated with an experiment",
 		Example: "  abs experiments notes list 23028",
+		Args:    cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			if len(args) != 1 {
-				cmd.Println("Usage:")
-				cmd.Println("  " + cmd.UseLine())
-				return fmt.Errorf("accepts 1 arg(s), received %d", len(args))
-			}
-
 			client, err := cmdutil.GetExpctldClient()

---

136-138: Success message is misleading when a condition filter is applied.

When --condition action=start is used, the message still reads "All notes deleted for experiment X", which could confuse users into thinking every note was removed.

Suggested fix

-			printer.Success(fmt.Sprintf("All notes deleted for experiment %s", args[0]))
+			if conditionMap != nil {
+				printer.Success(fmt.Sprintf("Matching notes deleted for experiment %s (condition: %s)", args[0], condition))
+			} else {
+				printer.Success(fmt.Sprintf("All notes deleted for experiment %s", args[0]))
+			}

---

187-209: Consider a small helper to reduce the repeated date-formatting block.

The same nil-check-then-format pattern appears three times (lines 189-200). A tiny local closure would tighten this up.

Suggested fix

+	fmtDate := func(t *time.Time) string {
+		if t == nil {
+			return "N/A"
+		}
+		return t.Format("2006-01-02")
+	}
+
 	table := output.NewTableData("ITERATION", "STATE", "CREATED", "STARTED", "STOPPED", "NOTES COUNT")
 	for i, entry := range timeline.Entries {
-		createdAt := "N/A"
-		if entry.CreatedAt != nil {
-			createdAt = entry.CreatedAt.Format("2006-01-02")
-		}
-		startedAt := "N/A"
-		if entry.StartedAt != nil {
-			startedAt = entry.StartedAt.Format("2006-01-02")
-		}
-		stoppedAt := "N/A"
-		if entry.StoppedAt != nil {
-			stoppedAt = entry.StoppedAt.Format("2006-01-02")
-		}
 		table.AddRow(
 			strconv.Itoa(i+1),
 			entry.State,
-			createdAt,
-			startedAt,
-			stoppedAt,
+			fmtDate(entry.CreatedAt),
+			fmtDate(entry.StartedAt),
+			fmtDate(entry.StoppedAt),
 			strconv.Itoa(len(entry.Notes)),
 		)
 	}

Note: this would require adding "time" to the imports.

---

183-185: Nit: duplicated output.Format(outputFormat) cast.

The conversion is evaluated twice in the same condition. Storing it in a variable improves readability.

-	if output.Format(outputFormat) != output.FormatTable && output.Format(outputFormat) != output.FormatPlain {
+	format := output.Format(outputFormat)
+	if format != output.FormatTable && format != output.FormatPlain {

README.md (1)

2104-2119: Profile configuration example shows API keys in plaintext YAML.

The example config snippet includes api_key: prod_key_here and api_key: staging_key_here. Given the CLI supports keyring-based credential storage, consider noting that tokens should be stored via abs auth login (which uses the keyring) rather than in the config file, or at minimum add a comment warning that this is insecure.

cmd/experiments/experiments_extra_test.go (4)

744-749: TestSearchHelpers does not test any project search helper — it only asserts stdlib behaviour.

The test name implies it exercises search helper functions, but it only calls strings.Contains / strings.ToLower on struct fields. Either invoke the actual search helper being tested (e.g., the matching logic inside runSearch) or remove this test.

---

1-21: Imports look correct; note bytes becomes unused if the manual stdout capture is replaced.

If the os.Pipe approach in TestGenerateTemplateCommand is replaced with testutil.CaptureStdout (as suggested), the "bytes" import on line 4 will become unused and should be removed.

---

30-173: newAPIServer route matching relies on case ordering in the switch statement.

The route-matching logic works correctly because more-specific suffix checks (e.g., /activity, /results, /start, /stop) precede the generic GET /experiments/{id} and PUT /experiments/{id} cases. However, this is fragile — inserting a new case in the wrong position could silently shadow an existing route.

Consider adding a brief comment near the top of the switch (e.g., // Order matters: specific suffixes before generic path matches) to make this constraint explicit for future maintainers.

---

239-256: Two distinct scenarios (empty list, table output) crammed into one test function.

TestRunListEmptyAndTable tests two independent flows: an empty-result list and a populated table-format list. Splitting into subtests (t.Run(...)) would improve failure diagnostics and make each scenario independently runnable.

cmd/experiments/experiments_extra_test.go (3)

496-503: Use testutil.CaptureStdout instead of manual os.Pipe plumbing.

testutil.CaptureStdout is already used at line 315 in this same file. The manual pipe-based stdout capture here is inconsistent and more error-prone (e.g. the error from os.Pipe() on line 497 is silently discarded).

♻️ Proposed refactor

-	var stdout bytes.Buffer
-	orig := os.Stdout
-	r, w, _ := os.Pipe()
-	os.Stdout = w
-	require.NoError(t, runGenerateTemplate(cmd, nil))
-	_ = w.Close()
-	_, _ = stdout.ReadFrom(r)
-	_ = r.Close()
-	os.Stdout = orig
-	assert.Contains(t, stdout.String(), "My Name")
+	output := testutil.CaptureStdout(t, func() {
+		require.NoError(t, runGenerateTemplate(cmd, nil))
+	})
+	assert.Contains(t, output, "My Name")

---

744-749: TestSearchHelpers only tests stdlib string operations, not project search logic.

Similar to the stdlib smoke tests flagged at lines 588–592, this test constructs an api.Experiment but only asserts strings.Contains/strings.ToLower behaviour — no project helper or search function is exercised. Replace with assertions that call the actual search helpers from the production code, or remove.

---

30-173: Test server route matching uses broad prefix/suffix checks that could overlap.

For instance, a path like /experiments/123/foo/results would match the results case at line 63 even though it's not a valid route. In test code this is unlikely to cause issues, but using more precise path matching (e.g. a regex or splitting on /) would guard against accidental route collisions as more tests are added.

cmd/experiments/notes_test.go (1)

21-25: Happy-path test doesn't verify the DELETE request was actually sent.

For the "valid condition" case, the test only asserts err == nil. If RunE were to skip the API call silently (e.g. due to a short-circuit bug), this test would still pass. Consider having the mock handler record whether it was invoked and asserting on that for the success case.

♻️ Suggested approach

+		var called bool
 		{
 			name:      "valid condition",
 			condition: "action=start",
 			expectErr: false,
 		},

Then in the handler for the valid case, set a flag:

handler: func(w http.ResponseWriter, r *http.Request) {
    called = true
    w.WriteHeader(http.StatusNoContent)
},

And after RunE:

if !tt.expectErr {
    require.NoError(t, err)
    assert.True(t, called, "expected DELETE request to be made")
}

Since the handler is shared across all cases, you could scope the called flag per-subtest or use an atomic.Bool.

Also applies to: 72-79

cmd/config/config.go (1)

394-406: Injectable package-level vars are a pragmatic choice for testability.

This pattern works well for a CLI tool. Just be aware that it makes these functions effectively global mutable state, which could cause issues if tests run in parallel without proper isolation.

cmd/metrics/metrics_validation_test.go (1)

33-35: Consider verifying the request body sent to the mock server.

The test confirms no error is returned, but does not assert that the correct payload (e.g. {"description":"updated"}) was actually sent to the server. This is a minor gap — adding a body assertion in the handler would make the test more robust against silent regressions.

cmd/goals/goals_validation_test.go (1)

33-35: Same suggestion: consider asserting the request body in the mock handler.

As noted in the metrics test, verifying the payload sent to the server would strengthen this integration test.

cmd/goals/goals_validation_test.go (1)

35-35: Don't discard the error from Flags().Set.

If the "name" flag is ever renamed or removed, discarding this error will cause the test to run with no flag set, masking the real failure.

Proposed fix

-	_ = updateCmd.Flags().Set("name", "updated")
+	require.NoError(t, updateCmd.Flags().Set("name", "updated"))

cmd/config/config.go (1)

143-192: Extract duplicated isSensitive check into a helper.

The same sensitive-key detection logic appears twice in runSet (lines 148–151 and 181–184). Extracting it into a small helper improves DRY compliance and makes it easier to keep the criteria consistent if more patterns are added later.

♻️ Suggested helper

+func isSensitiveKey(key string) bool {
+	k := strings.ToLower(key)
+	return strings.Contains(k, "token") ||
+		strings.Contains(k, "key") ||
+		strings.Contains(k, "secret") ||
+		strings.Contains(k, "password")
+}

Then replace both inline blocks with isSensitiveKey(key).

cmd/config/config.go (1)

143-192: Extract duplicated isSensitive logic into a helper.

The same four strings.Contains checks appear at lines 148–151 and again at lines 181–184. Extracting a small helper would reduce duplication and make it easier to keep the list of sensitive key patterns consistent.

Proposed refactor

+func isSensitiveKey(key string) bool {
+	lower := strings.ToLower(key)
+	return strings.Contains(lower, "token") ||
+		strings.Contains(lower, "key") ||
+		strings.Contains(lower, "secret") ||
+		strings.Contains(lower, "password")
+}
+
 func runSet(cmd *cobra.Command, key, inputValue string) error {
 	value := inputValue
 
 	// If value looks like it might need masking, prompt securely
 	if value != "" {
-		isSensitive := strings.Contains(strings.ToLower(key), "token") ||
-			strings.Contains(strings.ToLower(key), "key") ||
-			strings.Contains(strings.ToLower(key), "secret") ||
-			strings.Contains(strings.ToLower(key), "password")
-
-		if isSensitive && value == "prompt" {
+		if isSensitiveKey(key) && value == "prompt" {
 			...
 		}
 	}
 	...
 	// Mask sensitive values in output
-	isSensitive := strings.Contains(strings.ToLower(key), "token") ||
-		strings.Contains(strings.ToLower(key), "key") ||
-		strings.Contains(strings.ToLower(key), "secret") ||
-		strings.Contains(strings.ToLower(key), "password")
-
 	displayValue := value
-	if isSensitive {
+	if isSensitiveKey(key) {
 		displayValue = "****"
 	}